Skip to content

Add PreAllocatedOverlapped.UnsafeCreate / UnsafeAllocateNativeOverlapped - #53196

Merged
stephentoub merged 1 commit into
dotnet:mainfrom
stephentoub:unsafeoverlapped
May 25, 2021
Merged

Add PreAllocatedOverlapped.UnsafeCreate / UnsafeAllocateNativeOverlapped#53196
stephentoub merged 1 commit into
dotnet:mainfrom
stephentoub:unsafeoverlapped

Conversation

@stephentoub

Copy link
Copy Markdown
Member

Fixes #42549

@ghost

ghost commented May 24, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #42549

Author: stephentoub
Assignees: -
Labels:

area-System.Threading

Milestone: 6.0.0

@ghost

ghost commented May 24, 2021

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@GrabYourPitchforks

Copy link
Copy Markdown
Member

@stephentoub Feel free also to give feedback on the approved API shape. Anything weird? Missing? Some of the review was us trying to channel your spirit and proactively guess your feedback. :)

@stephentoub

Copy link
Copy Markdown
Member Author

Feel free also to give feedback on the approved API shape. Anything weird? Missing? Some of the review was us trying to channel your spirit and proactively guess your feedback. :)

My one piece of feedback is that I'm skeptical the UnsafeAllocateNativeOverlapped method is worthwhile. If you care about perf, you should be using PreAllocatedOverlapped... the UnsafeCreate there makes sense. But the minor savings that come from not capturing the ExecutionContext with the non-preallocated variant will pale in comparison to using the preallocated variant. Note that this PR doesn't make use of UnsafeAllocateNativeOverlapped, because our higher-performance code paths use PreAllocatedOverlapped.

@stephentoub
stephentoub merged commit d533fbc into dotnet:main May 25, 2021
@stephentoub
stephentoub deleted the unsafeoverlapped branch May 25, 2021 21:49
@ghost ghost locked as resolved and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

API Proposal: Add ThreadPoolBoundHandle.AllocateUnsafeNativeOverlapped

2 participants